(FYI: This PR is tiny if you suppress whitespace changes.)

This still seems wrong to me. The offsets have already been clamped prior to this, so no additional clamping should have been necessary. The strokeHalfWidth adjustment should just be accounting for the difference between a PatBlt rendering and a LineTo rendering. If it was correct, then the LineTo rendering should exactly match where the PatBlt would have rendered without that adjustment. If it ends up out of bounds, then that's clearly not the case, so something must be wrong with the strokeHalfWidth calculation. Maybe a rounding issue? I don't know.

Worst case, if you can't get it to work, at least move the strokeHalfWidth adjustment higher up, prior to where we're doing the first clamping (and get rid of this second set of clamping). Because we've got special case handling for the double underline offset, which is potentially going to be broken if you clamp it a second time.

I think another potential problem is that the maxUnderlineOffset that we're using for clamping is intended to be used with the original PatBlt offsets. Once you add the strokeHalfWidth adjustments, that range is no longer applicable. The only way I can see this working is if we can get that strokeHalfWidth calculation correct.

I'd suggest writing a little test app that renders a bunch of lines of varying widths, using both LineTo and PatBlt. From that you should be able to figure out exactly what offset needs to be applied at each width to get the two renderings to match.

Unfortunately, I'm not well-versed in Windows programming in general, so creating an app from scratch would be a different task for me 😅

However, I did experiment with the idea of comparing LineTo and PatBlt, and it made it much more clear what I needed to do.

With this code:

// a fixed offset
const auto cellMidY = std::lround(fontHeight / 2.0f);

// adjust for drawing with DrawStrokedLine()
const auto cellMidYWithHalfStrokeWidth = cellMidY + gsl::narrow_cast<int>(std::floor(_lineMetrics.underlineWidth / 2));

// Select a brush

// draw a line with PatBlt()
RETURN_HR_IF(E_FAIL, !DrawLine(ptTarget.x, ptTarget.y + cellMidY, widthOfAllCells, _lineMetrics.underlineWidth));

// Select the dotted pen

// draw a line with LineTo()
RETURN_HR_IF(E_FAIL, !DrawStrokedLine(ptTarget.x, ptTarget.y + cellMidYWithHalfStrokeWidth, widthOfAllCells));

Result:

"`e[4:4m Dotted `e[m"

I tried this with other font sizes, and they all show the same result. So, the DrawLine() and DrawStrokedLine() calls with those args are synonymous to each other.

This still seems wrong to me. The offsets have already been clamped prior to this, so no additional clamping should have been necessary.

Yeah, you're right! It wouldn't have been necessary if I had floored strokeHalfWidth before adding it to the two offsets. (I had lround'd it, so that was the issue.)

I found that this behavior can look somewhat unpleasant when Ctrl+scrolling up and down when using Consolas, because it'll jump in between very squiggly lines and barely squiggly lines all the time. Additionally, and unrelated to your PR, I'm having trouble ever getting the double-underline to separate into two distinct lines.

So, I'm currently doing a couple modifications to this function to bring it closer to the corresponding AtlasEngine code: Using floats throughout the function, unless rounding is needed (= for instance, right before calculating the thin line width) and I'm porting Word's double underline algorithm over. My hope is that all this will allow me to better understand how your current code works and to come up with an idea for making the transition between curly and less-curly lines "smoother". (I'll submit the above changes in a different PR after this one merges.)

Personally speaking I don't agree with this:

I know we can't move it upwards because that would mean it is discontinuous with the normal underline styles, so this seems totally sufficient to me!

I think discontinuous lines are better, if it avoids clipping. But I also get that discontinuous lines are kind of weird too.

So, I'm currently doing a couple modifications to this function to bring it closer to the corresponding AtlasEngine code.....(I'll submit the above changes in a different PR after this one merges.)

That sounds good to me.

I have a few ideas that I'll just share here:

1. We could give users an option to let WT/Conhost adjust underline-offfset. We'll make sure curlyline is always drawn perfectly. This is what Wezterm seems to do (can't be turned off).
2. (Not sure if it works but) Have a separate render pass for drawing underlines after glyphs/background have been drawn. This ensures we can draw underlines at the right offset while avoiding any clipping. (Too much work)

I'm having trouble ever getting the double underline to separate into two distinct lines.

j4james briefly mentioned it in his comment in the original PR that brought doubly underlined rendering support:

Note how some fonts only have enough space for a thicker line, and not two distinct lines, but I think that's OK (XTerm always renders the attribute this way). Also note that this is more common in the GDI renderer because it uses a smaller line height.

I've just had a chat with Dustin, and we compared different approaches. Word's double-underline algorithm does look fairly well at small and big font sizes and works better with Consolas. Using the thinLineWidth (as in AtlasEngine) for the curly underline also appears to be slightly better visually than the full underlineWidth.

I'll approve this PR because it puts this code in a fantastic state to do further "fine tuning" on it. Thank you so much for working on this! I also didn't want to ask you to make any more changes, since I don't want to put any more unnecessary burden on you. I'll link the follow-up PR here and post screenshots there to compare the changes.

Done: #16475